Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify createDerivedStore in debug.derived.ts #1801

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

dskloetd
Copy link
Contributor

@dskloetd dskloetd commented Feb 1, 2023

Motivation

Small optimization.
If all we want is a Readable view on a writable store, that doesn't expose set and update, creating a derived store is overkill.
The derived store subscribes to the original and forwards the changes but this is not necessary.

Changes

Change createDerivedStore to simply return an object with the same subscribe method and no other methods.

Tests

Refactoring should be covered by existing tests.

@dskloetd dskloetd force-pushed the kloet/readable-store branch from bdd4b9b to 0afb9d5 Compare February 2, 2023 07:15
@dskloetd dskloetd requested a review from lmuntaner February 2, 2023 07:20
Copy link
Contributor

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have tests for this feature.

Did you try that it worked by:

  • Go to Wallet page.
  • Click six times in the header.
  • Enter "fo" in the prompt.
  • Does the JSON file have the data of the "selectedAccount"?

@dskloetd dskloetd marked this pull request as ready for review February 2, 2023 08:31
@dskloetd
Copy link
Contributor Author

dskloetd commented Feb 2, 2023

Actually, I forgot to bind the method to the object so I expect it won't work.
I'll pause this until I can add a test that would have caught it.

@dskloetd dskloetd marked this pull request as draft May 8, 2023 11:10
@anedos-dfinity
Copy link
Contributor

Should this PR be closed ?

@lmuntaner
Copy link
Contributor

@dskloetd what do you think?

@dskloetd
Copy link
Contributor Author

Does it matter? It's marked as Draft already.

@bitdivine bitdivine closed this Aug 14, 2023
@bitdivine bitdivine deleted the kloet/readable-store branch August 14, 2023 15:33
@dskloetd dskloetd restored the kloet/readable-store branch August 14, 2023 15:35
@dskloetd dskloetd reopened this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants